-
Notifications
You must be signed in to change notification settings - Fork 43
feat(c/sedona-proj): Implement item crs support for ST_Transform #531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9570f39 to
3c13875
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements item-level CRS (Coordinate Reference System) support for the ST_Transform function, enabling transformations where the target CRS varies per row rather than being fixed at the type level.
Changes:
- Adds support for item-level CRS input/output in
ST_Transform, allowing per-row CRS specifications - Refactors argument handling to support both scalar and array CRS inputs with appropriate return type determination
- Optimizes transformation by caching when both source and target CRS are constant across all rows
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| rust/sedona-testing/src/testers.rs | Updates invoke method to compute return types with scalar arguments |
| rust/sedona-functions/src/executor.rs | Adds support for iterating over item-crs struct arrays by extracting geometry from the "item" field |
| c/sedona-proj/src/st_transform.rs | Major refactor to support item-crs inputs/outputs with new argument pattern matching and transformation logic |
| c/sedona-proj/Cargo.toml | Adds sedona-common dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| match maybe_wkb { | ||
| Some(wkb) => { | ||
| invoke_scalar(&wkb, crs_transform.as_ref(), &mut builder)?; | ||
| builder.append_value([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to confirm the builder.append_value([]); is expected to be called after the invoke_scalar call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is expected (invoke_scalar writes into the builder directly but the append_value is needed to force a new element from the builder).
| executor.execute_wkb_void(|maybe_wkb| { | ||
| match (maybe_wkb, crs_to_crs_iter.next().unwrap()) { | ||
| (Some(wkb), (Some(from_crs_str), Some(to_crs_str))) => { | ||
| let maybe_from_crs = deserialize_crs(from_crs_str)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe caching could help here since this is per-row calculation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a thread local cache behind deserialize_crs() for this reason!
...there are probably more ways we could speed up per-row CRS operations once they actually work, though.
This PR integrates Item Crs inputs and outputs into ST_Transform, which now has a rather complicated signature with respect to what can happen:
There's probably no great way to implement all of that cleanly but I did try to reduce repetition as much as possible.